feat(cms): Tier 1 behavioral fuzzers + fix two RFC 5652 bypasses#13
Merged
Conversation
Adds Tier-1 behavioral fuzzers and fixes two verifier bypasses both discovered within seconds of running them. New fuzzers (pkg/cms/behavioral_fuzz_test.go): - FuzzSignVerifyRoundtrip: sign+verify+tamper invariant for Case 1 (with signedAttributes). - FuzzSignDataWithoutAttributesRoundtrip: same for Case 2. - FuzzSignDataWithSignerRoundtrip: crypto.Signer abstraction (PR #6). - FuzzCase2SignDeterminism: confirms Ed25519's deterministic-signature property propagates through the CMS encoder for Case 2. Findings (crash corpora preserved under pkg/cms/testdata/fuzz/ as regression seeds): 1. SignedData.Version was unchecked. Any int — including negatives produced by single-byte tampering of the INTEGER payload (0x01 -> 0x81 decodes as -127) — was accepted. RFC 5652 §5.1 constrains the value to {1, 3, 4, 5}. 2. EncapContentInfo.eContentType was unchecked on the Case 2 path (no signedAttributes). An attacker could flip bytes inside the eContentType OID without invalidating the signature, because there was no contentType signed attribute to cross-check against. RFC 5652 §11.1 requires eContentType == id-data when signedAttributes are absent. Both fixes are minimal additions in parseSignedData and verifyMessageDigest respectively. After fixes, ~20M cumulative fuzz iterations across the four new fuzzers (60s each) produced no further findings. Full test suite passes under -race with 75.3% coverage. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens CMS verification correctness by adding behavioral fuzz coverage and tightening validation for two RFC 5652 verifier bypass cases.
Changes:
- Rejects invalid
SignedData.Versionvalues outside{1, 3, 4, 5}. - Requires
EncapContentInfo.ContentTypeto beid-datawhen signed attributes are absent. - Adds behavioral fuzzers and regression corpus seeds for sign/verify roundtrips, Case 2 signatures,
crypto.Signer, and determinism.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
pkg/cms/verifier.go |
Adds RFC validation for SignedData version and Case 2 content type. |
pkg/cms/behavioral_fuzz_test.go |
Adds Tier 1 behavioral fuzz tests for signing and verification contracts. |
pkg/cms/testdata/fuzz/FuzzSignVerifyRoundtrip/bbfdccec14dad20a |
Adds regression seed for invalid SignedData version tampering. |
pkg/cms/testdata/fuzz/FuzzSignDataWithoutAttributesRoundtrip/48d0f8f6cbb39a02 |
Adds regression seed for Case 2 content type tampering. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Continues the audit-equivalent hardening on this branch. Findings (1 fix in this commit): - parseASN1Length accepted non-canonical DER length encodings (long-form for values < 128, long-form with leading zero bytes). RFC 5652 §10.1 mandates DER for SignedAttributes; accepting BER alternates created a malleability surface where a single CMS message could be re-encoded into structurally distinct but cryptographically valid forms, breaking content-addressing built atop the CMS blob hash. Fix: explicit canonicality checks in parseASN1Length, covered by new unit tests. New audit-style tests (pkg/cms/tamper_enum_test.go): - TestEnumerateUnsignedBytesCase1 / Case2: byte-by-byte XOR-0xff sweep over a valid CMS, recording every position whose modification still passes Verify. After the parseSignedData and verifyMessageDigest fixes in the previous commit, *both Case 1 (597 bytes) and Case 2 (455 bytes) have zero surviving unsigned positions* — every byte of the signed structure is load-bearing under the strict verifier. The test caps the allowed surface at 8 so future regressions are caught. New Tier 2 fuzzers (pkg/cms/tier2_fuzz_test.go): - FuzzInsertByte / FuzzDeleteByte: structural-mutation fuzz; verify must reject any insertion/deletion at any position. - FuzzAppendTrailingData: trailing-data rejection (parseContentInfo already enforces this; the fuzzer locks the invariant in). - FuzzCertBagSubstitution: produces two independent signing pairs, signs with A, swaps A's cert DER with B's inside the CMS blob, asserts Verify rejects under both trust topologies. The canonical "wrong cert in cert bag" attack — a bug here would be catastrophic. - FuzzVerifyAcceptsOnlyCanonicalForm: two-byte tamper invariant — if Verify accepts the tampered blob, the returned cert MUST match the original signer cert byte-for-byte. Detects substitution forgeries. New concurrency tests (pkg/cms/concurrency_test.go, run under -race): - TestVerifyConcurrent: 32 goroutines × 200 verifications against the same blob; all must succeed and return the same cert. - TestSignConcurrent: deterministic Case 2 outputs must be byte-identical across goroutines; concurrent Case 1 outputs must each independently verify. Maintenance: - Removed four unused ASN.1 tag constants (tagSequence, tagOctetString, tagInteger, tagBitString) flagged by the language server. Validation: full suite passes under -race with 77.9% coverage (+2.6 points from new branch coverage). Tier 1 and Tier 2 fuzzers ran ~26M cumulative iterations after the DER fix with no further findings. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Algorithm-OID malleability: - FuzzReplaceOIDBytes locates the Ed25519 signature-algorithm OID (06 03 2b 65 70) inside the CMS blob and overwrites the value bytes with fuzzer-supplied alternates. Verify must reject every variant; accepting any substitution would mean an attacker can claim a signature was produced under a different algorithm than it was. Signature-region surgery: - TestSignatureRegionSurgery isolates the 64-byte Ed25519 signature OCTET STRING and replaces it with all-zero, all-0xff, first-byte flipped, and last-byte flipped variants. Verify must reject every one — this tests the cryptographic check in isolation from the structural/encoding checks. DoS / resource exhaustion: - FuzzDeclaredLengthOverflow seeds the parser with short blobs whose outer SEQUENCE declares 16 MB to 4 GB of inner content. parseASN1Length must reject in bounded time without OOM or hang. ~7M execs in 20s, clean. Cert-bag handling: - TestVerifyEmptyCertBag zero-fills the embedded cert DER; Verify must reject because no signer cert can be recovered. - TestVerifyWithExtraTrustedCert puts an unrelated trusted cert in the pool alongside the actual signer; Verify must return signer A's cert, not the bystander, when validating A's signature. All unit tests pass deterministically (verified with -count=10 against the previously-flaky truncated-sig case, which has been replaced with deterministic XOR-flip variants). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Security scan hardening: - Remove the global -exclude=G115 from gosec. The flag was a workaround for ASN.1 DER length-encoding code, but a full scan now yields 0 issues — keeping the mask in place would hide real future findings. If a legitimate G115 site appears later, suppress it locally with a `// #nosec G115 -- reason` annotation, never globally. - Add a govulncheck step to surface stdlib/dependency CVEs reachable from the package's call graph. Currently 0 reachable vulnerabilities. Per-PR fuzz coverage expansion: - Extend the "Run fuzz tests (short)" step to include the four highest-signal new fuzzers (FuzzSignVerifyRoundtrip, FuzzSignDataWithoutAttributesRoundtrip, FuzzCertBagSubstitution, FuzzReplaceOIDBytes). 10s each in CI; the test-suite-wide replay of committed crash corpora still happens via `go test ./...`. Validation: - 47.6M-execution 3-minute local burn-in of FuzzVerifyAcceptsOnlyCanonicalForm (the strongest cross-region invariant we have) yields zero findings on top of the in-PR fixes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
govulncheck reported 11 reachable Go standard-library vulnerabilities when run against go 1.25.1, including: - GO-2025-4011 (encoding/asn1: parse-induced memory exhaustion) — reachable via cms.verifyMessageDigest → asn1.Unmarshal - GO-2025-4155 (crypto/x509: excessive resource consumption in error printing) — reachable via cms.verifyCertificateChain → Verify - GO-2025-4013 (crypto/x509: DSA-pubkey panic on cert validation) — same path - GO-2025-4010 (net/url: invalid bracketed IPv6 parsing) — transitive via x509.Certificate.Verify - GO-2025-4007 (crypto/x509: quadratic name-constraint check) — same - GO-2025-4009 (encoding/pem: quadratic parse) — via cmd/cms-test-tool All are fixed in go ≥ 1.25.5. Bumping go.mod's go directive to 1.25.5 auto-toolchain-upgrades older CI matrix entries to 1.25.5, so the matrix continues to serve as a portability smoke test while every effective build runs on the CVE-fixed stdlib. CI workflow updates: - Pin hardcoded job Go versions to '1.25' (latest 1.25.x patch) so future CVE fixes land in CI automatically without workflow edits. - Matrix entry '1.25.1' → '1.25' for the same reason; covers the references in `if: matrix.go-version == ...` guards too. Local validation: tests pass under -race; govulncheck reports 0 reachable vulnerabilities. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…AcceptsOnlyCanonicalForm (go test refuses multi-match)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit-equivalent hardening pass on
pkg/cms. Adds three tiers of behavioral fuzz coverage, byte-by-byte tamper enumeration, concurrency tests, and CI hardening — and fixes three RFC-5652 verifier bypasses surfaced by the new fuzzers.This is intentionally one larger PR rather than a sequence: each fuzzer's value is highest paired with the fix it motivated, and keeping them together keeps the audit trail coherent. The repo is marked experimental, so the priority is finding and fixing real bugs before tagging for review.
Findings & fixes (all fixed in this PR)
1.
SignedData.Versionaccepted anyint— RFC 5652 §5.1The verifier explicitly avoided enforcing
SignedData.Version. Any integer — including negatives produced by single-byte tampering of the DERINTEGERpayload — was accepted. Per RFC 5652 §5.1, valid versions are{1, 3, 4, 5}. The fuzzer found this by flipping byte 25 of a valid signature (the version field) from0x01to0x81(which decodes as-127).Fix: explicit whitelist in
parseSignedData.2.
EncapContentInfo.eContentTypeunchecked in Case 2 — RFC 5652 §11.1In Case 2 messages (no
signedAttributes), there is nocontentTypesigned attribute to cross-check against the outereContentType. The Case 2 verifier path returned early and never validated the OID. The fuzzer found this by flipping bytes inside theeContentTypeOID without invalidating the signature.Fix: when
signedAttributesare absent, requireeContentType == id-data.3.
parseASN1Lengthaccepted non-canonical DER lengths — RFC 5652 §10.1The internal
parseASN1Lengthhelper accepted long-form length encodings for values < 128 (e.g.,0x81 0x05for value 5) and long-form with leading zero bytes (e.g.,0x82 0x00 0x80for value 128). RFC 5652 §10.1 mandates DER for SignedAttributes; accepting BER alternates created a malleability surface where a single CMS message could be re-encoded into structurally distinct but cryptographically valid forms — breaking any content-addressing or duplicate-detection built atop the CMS blob hash.Fix: explicit canonicality checks in
parseASN1Length.What's added
Behavioral fuzzers — Tier 1 (
behavioral_fuzz_test.go)FuzzSignVerifyRoundtripFuzzSignDataWithoutAttributesRoundtripFuzzSignDataWithSignerRoundtripcrypto.Signer(PR #6) pathFuzzCase2SignDeterminismBehavioral fuzzers — Tier 2 (
tier2_fuzz_test.go)FuzzInsertByte/FuzzDeleteByteFuzzAppendTrailingDataFuzzCertBagSubstitutionFuzzVerifyAcceptsOnlyCanonicalFormBehavioral fuzzers — Tier 3 (
tier3_fuzz_test.go)FuzzReplaceOIDBytesFuzzDeclaredLengthOverflowAudit-style enumeration (
tamper_enum_test.go)Byte-by-byte XOR-0xff sweep over a valid CMS, recording every position whose modification still passes Verify. After the in-PR fixes, both Case 1 (597 bytes) and Case 2 (455 bytes) have zero surviving unsigned positions — every byte of the signed structure is load-bearing. The test caps the allowed surface at 8 to catch future regressions.
Targeted unit tests
TestSignatureRegionSurgery— isolated 64-byte Ed25519 signature surgery (all-zero, all-0xff, first-byte flip, last-byte flip).TestVerifyEmptyCertBag— cert-bag zero-fill must reject.TestVerifyWithExtraTrustedCert— extra bystander trusted cert must not enable attesting for unrelated signatures.TestParseASN1LengthRejectsNonCanonical+TestParseASN1LengthAcceptsCanonical— strict DER probe.Concurrency tests (
concurrency_test.go)TestVerifyConcurrent— 32 goroutines × 200 verifications under-race, all must succeed with consistent cert.TestSignConcurrent— deterministic Case 2 outputs byte-identical across goroutines; concurrent Case 1 outputs each independently verify.CI hardening (
.github/workflows/ci.yml)-exclude=G115fromgosec(full scan now yields 0 issues).govulncheckstep for stdlib/dependency CVE detection on call graph.Validation
-racewith 77.9% statement coverage (+2.6 from main).FuzzVerifyAcceptsOnlyCanonicalFormburn-in: 47.6M execs over 3 minutes, zero findings.-countrepetitions of the full suite under-race: stable.gosec(without G115 exclude): 0 issues.govulncheck: 0 reachable vulnerabilities.staticcheck: 0 issues.Test plan
-exclude=G115.pkg/cms/testdata/fuzz/) pass.-race.🤖 Generated with Claude Code